-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix empty string check in quiz question answers #7614
Conversation
It was failing with falsy values
Test the previous changes of this PR with WordPress Playground. |
Just closing and reopening to fix the milestone validation check |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works as expected. Just added one comment about the test name convention. Also Is the failing test relevant to this PR?
/** | ||
* Tests posting multiple choice question with empty string as an answer. | ||
*/ | ||
public function testPostMultipleChoiceQuestion_WhenAnswersContainsEmptyString_AnswerIsRemoved() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we are calling the send_post_request
method, it's probably calling the save_quiz method through the API. So the test is actually for the save_quiz
method. As per the naming convention doc here - p6rkRX-35r-p2, the name of the test should probably be testSaveQuiz
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review, Imran!
Fixed the test format here: 126cb95
I also added the "Arrange", "Act" and "Assert" comments that I had forgotten.
It's not. Just to confirm, I created another PR and it had the same issue: https://github.com/Automattic/sensei/actions/runs/9486813834/job/26142021297?pr=7617 |
Test the previous changes of this PR with WordPress Playground. |
Since the last changes were only related to tests format, I'm merging this for the coming release. @Imran92, if you see anything else in my change, let me know that I can open a new PR. 😉 |
Resolves #7606
Proposed Changes
Testing Instructions
Pre-Merge Checklist